Repository navigation
Conversation
Bun.generateHeapSnapshot("v8"), v8.writeHeapSnapshot(), v8.getHeapSnapshot()
and Worker#getHeapSnapshot() all called heapProfiler.clearSnapshots() before
constructing BunV8HeapSnapshotBuilder. That nulls m_previousSnapshot and
defeats the id-reuse logic the builder already implements: every surviving
object got a fresh id on every snapshot, so DevTools' Comparison view and
memlab saw the whole heap as freed-and-reallocated between any two snapshots.
Drop the clearSnapshots() calls so the builder can look up surviving cells in
the previous snapshot. Heap::removeDeadHeapSnapshotNodes() already prunes dead
entries from the retained chain on every GC.
|
Updated 4:10 PM PT - Jul 13th, 2026
❌ @robobun, your commit a7aa5f1 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34085That installs a local version of the PR into your bun-34085 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (3)
WalkthroughHeap snapshot entry points now preserve existing profiler snapshots while ensuring profiler initialization. A new fixture mode and test verify that V8 node IDs remain stable across multiple snapshots. ChangesHeap snapshot stability
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
CI build #72529: the new
Ready for review. |
There was a problem hiding this comment.
Beyond the inline note on the snapshot chain, I also checked the non-v8 Bun.generateHeapSnapshot() / "jsc" path in BunObject.cpp — removing clearSnapshots() there does not turn its output into a delta (JSC's HeapSnapshotBuilder::json() still emits the full node set), so the default format is unaffected.
Extended reasoning...
The three-line deletion is well-reasoned and the id-stability test covers both the string and arraybuffer v8 entry points. The main residual risk was that functionGenerateHeapSnapshot shares the removed clearSnapshots() call between the v8 and JSC-format branches, so the JSC HeapSnapshotBuilder now also sees a prior snapshot; verifiers confirmed its json() still serializes all live nodes rather than only the delta, so repeated Bun.generateHeapSnapshot() calls remain complete. Deferring rather than approving because the change alters HeapProfiler retention semantics (per-GC chain walk, inline nit) and lives in the JSC bindings — worth a maintainer glance.
|
Cost data for the retained snapshot chain that this PR depends on. I measured it while tracing a CI flake. The Cost section of #29557 prices the chain in bytes only, not in GC time. While a snapshot is registered in the VM's Measured on main (release build, linux x64). One snapshot is taken, nothing else changes:
Main pays this today and gets no stable ids for it. The clear-before calls that this PR removes run before the next build, and nothing clears after a build. This is the I reproduced it on an Intel Mac mini from the CI pool with the shard's own batch (769 files, Two changes in oven-sh/WebKit would keep the chain and remove most of the cost:
I had a Bun-side change that clears the snapshots after each build. I did not open it, because it removes the chain that this PR and #29557 want to keep. |
|
The first of the two fork changes from my comment above is now a PR: oven-sh/WebKit#656. Measured in the
It keeps the id chain, so it does not decide anything for this PR. It removes the eden cost that this PR would otherwise make permanent for every process that takes one V8 snapshot. The |
What
Heap-snapshot node ids are not stable across snapshots: the same live object gets a fresh id on every
v8.writeHeapSnapshot()/Bun.generateHeapSnapshot("v8")call. This breaks every heap-diff tool that keys on the V8 id-stability contract (DevTools Comparison view, memlab): between any two snapshots the entire heap appears freed-and-reallocated.Cause
functionGenerateHeapSnapshot(and the sibling v8 entry points inBunHeapProfiler.cpp/JSWorker.cpp) calledheapProfiler.clearSnapshots()before constructingBunV8HeapSnapshotBuilder. That nullsm_previousSnapshot, so the id-reuse lookup inanalyzeNodeInternalnever hits and every cell is assigned a freshHeapSnapshotBuilder::getNextObjectIdentifier():Fix
Drop the three
clearSnapshots()calls so the builder can find surviving cells in the previous snapshot.Heap::removeDeadHeapSnapshotNodes()already prunes dead entries from the retained chain on every GC, so a freed-and-reused address cannot alias an old id. The retainedHeapSnapshotonly stores cells that were new in that snapshot (delta storage), so repeated snapshots do not grow unboundedly.Verification
Added
id-stabilitymode to the v8 heap-snapshot fixture: tags an object onglobalThis, takes two snapshots (string and arraybuffer paths), and asserts the tagged object's id is identical in both.Before:
id1=888 id2=6824 stable=falseAfter:
id1=1234 id2=1234 stable=true[review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file